Codecov Report

❌ Patch coverage is 93.98907% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 43.88%. Comparing base (0710471) to head (9db7964).
⚠️ Report is 3 commits behind head on master.

@@            Coverage Diff             @@
##           master    #2715      +/-   ##
==========================================
+ Coverage   39.02%   43.88%   +4.86%     
==========================================
  Files         126      133       +7     
  Lines        5825     6189     +364     
  Branches     1546     1655     +109     
==========================================
+ Hits         2273     2716     +443     
+ Misses       3550     3469      -81     
- Partials        2        4       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Requirements

What is the PR trying to do? Is this the right thing? Are there bugs in the requirements?

Easy Checks

Authentication

• Never use tx.origin
• Every external/public function is supposed to be externally accessible
• Every external/public function has the correct authentication
• All initializers have onlyGovernor
• Each method that changes access control has the correct access control
• If the proxy & implementation approach is used & implementation extends Governable.sol, make sure governor on the implementation contract is explicitly set to address(0)

Ethereum

• Contract does not send or receive Ethereum.
• Contract has no payable methods.
• Contract is not vulnerable to being sent self destruct ETH
• If contract interacts with ETH make sure there are no read only reentrancies (like this one in Curve pools)

Cryptographic code

• This contract code does not roll it's own crypto.
• No signature checks without reverting on a 0x00 result.
• No signed data could be used in a replay attack, on our contract or others.

Black magic

• Does not contain selfdestruct
• Does not use delegatecall outside of proxying. If an implementation contract were to call delegatecall under attacker control, it could call selfdestruct the implementation contract, leading to calls through the proxy silently succeeding, even though they were failing.
• Address.isContract should be treated as if could return anything at any time, because that's reality.

Overflow

• Code is solidity version >= 0.8.0

License

• The contract uses the appropriate limited BUSL-1.1 (Business) or the open MIT license
• If the contract license changes from MIT to BUSL-1.1 any contracts importing it need to also have their license set to BUSL-1.1

Proxy

• No storage variable initialized at definition when contract used as a proxy implementation.
• Any added storage slots are after existing slots.
• Any added inheritance does not affect storage slots for upgradable contracts.

Events

• All state changing functions emit events https://github.com/OriginProtocol/origin-dollar/pull/2715/changes#r2694015741

Medium Checks

Rounding and casts

• Contract rounds in the protocols favor
• Contract does not have bugs from loosing rounding precision
• Code correctly multiplies before division
• Contract does not have bugs from zero or near zero amounts
• Safecast is aways used when casting

Dependencies

• Review any new contract dependencies thoroughly (e.g. OpenZeppelin imports) when new dependencies are added or version of dependencies changes.
• If OpenZeppelin ACL roles are use review & enumerate all of them.
• Check OpenZeppelin security vulnerabilities and see if any apply to current PR considering the version of OpenZeppelin contract used.

External calls

• Contract addresses passed in are validated
• No unsafe external calls
• Reentrancy guards on all state changing functions
  • Still doesn't protect against external contracts changing the state of the world if they are called.
• No malicious behaviors
• Low level call() must require success.
• No slippage attacks (we need to validate expected tokens received)
• Oracles, one of:
  • No oracles
  • Oracles can't be bent
  • If oracle can be bent, it won't hurt us.
• Do not call balanceOf for external contracts to determine what they will do when they use internal accounting

Deploy

• Deployer permissions are removed after deploy

Strategy Specific

Strategy checks

• Check balance cannot be manipulated up AND down by an attacker - Balance can be manipulated with a USDC transfer.
• No read only reentrancy on downstream protocols during checkBalance
• All reward tokens are collected
• The harvester can sell all reward tokens
• No funds are left in the contract that should not be as a result of depositing or withdrawing
• If the strategy deals with staking LP tokens any liquidity altering function: deposit, depositAll, withdraw, withdrawAll or custom (e.g. rebalance) should result in a state where all LP tokens owned by the contract remain staked
• All funds can be recovered from the strategy by some combination of depositAll, withdraw, or withdrawAll()
• WithdrawAll() can always withdraw an amount equal to or larger than checkBalances report, even in spite of attacker manipulation.
• WithdrawAll() cannot be MEV'd
• WithdrawAll() does not revert when strategy has 0 assets
• Strategist cannot steal funds

Thinking

Logic

Are there bugs in the logic?

No

• Correct usage of global & local variables. -> they might differentiate only by an underscore that can be overlooked (e.g. address vs _address).

Resource usage

The contract interacts with CCTP contracts (sends/receives messages&USDC), ERC-4626 vaults from Morpho (deposits/withdraws assets for shares), and the OUSD Vault (transfers USDC).

Internal State

The contract enforces that only one transfer can be pending at a time, with pendingAmount tracking in-flight deposits and remoteStrategyBalance caching the remote chain's balance. Before any deposit or withdrawal, the previous transfer must be complete, amounts must be at least 1e6 USDC, and withdrawals cannot exceed the cached remote balance. After a successful round-trip, pendingAmount should reset to zero and remoteStrategyBalance should reflect reality.

Attack

Code assumes the operator is capable of fixing potential issues with the vaults or CCTP if the operator is not alive funds can be stucked, no outside attack vector.

Flavor

Code would benefit from a secondary cross chain relayer but this added complexity is not worth the risk of CCTP bridge being down.